Skip to content

Show the psnr reported by optimize and convert - #45

Merged
mathiasgrimm merged 3 commits into
mainfrom
feat/psnr-output
Aug 2, 2026
Merged

Show the psnr reported by optimize and convert#45
mathiasgrimm merged 3 commits into
mainfrom
feat/psnr-output

Conversation

@mathiasgrimm

Copy link
Copy Markdown
Owner

CLI side of Art-Commerce-Systems/glimpseimg.com#91 (API PR: Art-Commerce-Systems/glimpseimg.com#98, SDK PR: mathiasgrimm/glimpse-php#11).

What

  • GlimpseCommand::emit() appends , PSNR 41.27 dB to the human summary and adds a psnr key to --json output, but only when the SDK reports a non-null value. Resize and thumbnail (and older APIs) yield null from the SDK, so their output is byte-identical to today, locked by the existing exact-match JSON test plus a new null-omission test.
  • fakeTransformResponse() / fakeTransform() gain an overrides parameter, following the fakeInfoResponse merge pattern.
  • composer.json bumps mathiasgrimm/glimpse-php to ^3.1 (the ImageResult->psnr property).

Merge order (CI is red until then, expected)

  1. API PR merges and deploys.
  2. glimpse-php PR merges and v3.1.0 is tagged (Packagist updates via webhook).
  3. Then on this branch: composer update mathiasgrimm/glimpse-php, commit the lock, CI goes green, merge, release v1.4.0 (make release bundles the updated vendor into the phar).

Until step 2 the locked SDK 3.0.0 has no psnr property, so the new tests fail on CI. Locally the suite was run against the SDK branch and passes: 288 tests, 800 assertions, Pint and PHPStan clean.

Follow-up (out of scope, per the plan on the issue): persisting psnr into the baseline file.

🤖 Generated with Claude Code

mathiasgrimm and others added 2 commits July 22, 2026 10:04
The API now returns psnr for optimize and convert. When it is present
the human summary gains a ", PSNR 41.27 dB" suffix and the --json
output gains a psnr key; when it is null (resize, thumbnail, no
measurable loss, older API) the output is unchanged.

Requires mathiasgrimm/glimpse-php ^3.1 for the ImageResult psnr
property; the lock stays on 3.0.0 until v3.1.0 is tagged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Format the PSNR summary with %.2F so a non-C LC_NUMERIC cannot turn
  the decimal point into a comma.
- Lock the "resize carries no psnr" contract with an exact --json test
  on the resize command, and assert the human summary has no PSNR when
  the API reports null.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mathiasgrimm

Copy link
Copy Markdown
Owner Author

Two independent reviews, one by a Claude agent and one by codex (gpt-5.6-sol, read-only). Both approve the behavior; codex requested one change (locale-dependent formatting). Addressed in commit d39c2fd, followed by both reviews.

What was addressed

  • Locale, codex medium. The human summary used sprintf('%.2f', ...), which prints a comma decimal under a non-C LC_NUMERIC. Switched to %.2F (locale-independent), so the output is always PSNR 41.27 dB.
  • Per-command contract, both raised as minor. Resize and thumbnail had no --json test, so the "no psnr key" promise held only transitively through the shared emit(). Added an exact-match --json test on the resize command and a "no PSNR in the human summary when null" assertion on convert.

Left as noted follow-ups (not this PR): the SDK-version bump means CI stays red until glimpse-php v3.1.0 is tagged and the lock is refreshed (composer update mathiasgrimm/glimpse-php); the - stdout/STDERR routing has no test, but that gap predates this change and the PSNR path takes the same branch.


Claude review

Verdict: clean and correct; approve once glimpse-php v3.1.0 is tagged and the lock is refreshed.

What I verified

  • emit() correctness. JSON: psnr is appended after height only when non-null, using a strict !== null check, so a legitimate 0.0 still emits. Human summary: the suffix renders inside the existing parentheses and collapses to an empty string when null, so pre-PSNR output is byte-identical. The - output routing is untouched: both payload and summary are built before the STDERR-vs-line ternary, so PSNR flows identically to the STDERR path (metadata must not corrupt the image bytes on STDOUT).
  • Tests lock all three JSON states: psnr present (strict toBe including the key), explicit "psnr": null, and key absent (the pre-existing strict toBe). The human summary is locked with the full parenthesized string for both convert and optimize.
  • Suite: composer test passes against the patched vendor SDK: Pint clean, PHPStan clean, 288 tests / 800 assertions.
  • Constraint bump ^3.1 is right; ImageResult->psnr first exists in 3.1.

Findings

  • Minor: the "resize/thumbnail unchanged" claim is only locked transitively. Neither resize nor thumbnail has a --json test, and their summary assertions are prefix-only. One strict toBe JSON test on resize would pin the per-command contract.
  • Minor: the - output branch of emit() has no test (predates this PR).

Codex review

Verdict: Changes requested. The JSON behavior and STDERR routing are correct, but human output is locale-dependent.

Medium

  • app/Commands/GlimpseCommand.php uses locale-aware %.2f. Under LC_NUMERIC=de_DE.UTF-8, this renders PSNR 41,27 dB instead of the documented PSNR 41.27 dB. Use %.2F or another locale-independent formatter.

Low

  • Convert tests only exercise file output. No test invokes --output=-, where image bytes must stay on STDOUT and metadata on STDERR. The implementation is correct but this binary-stream contract is unprotected.

Minor notes

  • JSON output preserves the existing shape and conditionally adds a numeric psnr key only when non-null.
  • The explicit JSON null-omission test is good. Consider also asserting the human output contains no PSNR when null.
  • Resize and thumbnail tests only check a summary prefix; exact assertions would better protect the "unchanged" promise.
  • The ^3.1 constraint is the correct minimum.
  • Local verification passed: 288 tests, 800 assertions, plus Pint and PHPStan.

@mathiasgrimm
mathiasgrimm merged commit 73fbc1c into main Aug 2, 2026
5 checks passed
@mathiasgrimm
mathiasgrimm deleted the feat/psnr-output branch August 4, 2026 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant